1. What this PR does / why we need it:

This issue is validation of the Checkpoint when a client calls the PushPull API and sends a request ChangePack. This validation is necessary to handle potential issues such as network-induced duplicate requests, bugs in the new SDK, or malicious tampering.

Specifically, three scenarios needed to be validated:

1. The ClientSeqs in the reqPacks.Changes are sequential.
2. The ClientSeqs in the request is sequential with DocInfo.Checkpoint.ClientSeq.
3. The ClientSeq in the request sent to the cannot be less than the ClientSeq in the Server's ClientInfo. This addresses duplicate requests.
4. The ServerSeq in the request sent to the PushPull API cannot be greater than the ServerSeq maintained by the server. This addresses malicious requests.

I will refer to these three scenarios as Case 1, 2, 3, 4

Upon reviewing the code, I found that Cases 3 and 4 were already implemented.

Thus, I only needed to implement Case 1. However, I have written test codes for Cases 1, 2, 3, 4 and the happy case. I will explain the implementation for each case and the corresponding test code.

In the upcoming code blocks, the first line of each comment will indicate the file name and function name for your reference.

I also have a few questions. These questions are marked in bold and will be included in the Special notes for your reviewer section.

1.1 Case 1: The ClientSeqs in the request sent to the PushPull API are sequential

This validation is implemented in packs.go and occurs immediately when the PushPull function is called.

server/packs/packs.go

func validateClientSeqSequential(changes []*change.Change) error {
	if len(changes) <= 1 {
		return nil
	}

	nextClientSeq := changes[0].ClientSeq()
	for _, cn := range changes[1:] {
		nextClientSeq++

		if nextClientSeq != cn.ClientSeq() {
			return fmt.Errorf(
				"ClientSeq in Changes are not sequential (expected: %d, actual: %d) : %w",
				nextClientSeq,
				cn.ClientSeq(),
				ErrClientSeqNotSequential,
			)
		}
	}
	return nil
}

This function checks if the Changes in the request are sequential.

Q. It does not validate whether the first Change's ClientSeq differs by 1 from the ClientSeq in the existing ClientInfo. Should this be added? Logically, if there are 0 or 1 changes, there's nothing to validate, so it returns without issues. If the sequence is not sequential, it returns ErrClientSeqNotSequential, which is mapped to connect.CodeInvalidArgument in the response via errorToConnectCode.

server/packs/packs.go

func PushPull(
    ...
	// TODO: Changes may be reordered or missing during communication on the network.
	// We should check the change.pack with checkpoint to make sure the changes are in the correct order.
	err := validateClientSeqSequential(reqPack.Changes)
	if err != nil {
		return nil, err
	}
    ...

This validation is tested at the location of the above comment.

Q. Should we now remove this TODO comment?

1.2 `Case 2: The ClientSeq in changes not sequential with DocInfo.Checkpoint.ClientSeq

// packs/packs.go
func validateClientSeqSequentialWithCheckpoint(changes []*change.Change, checkpoint change.Checkpoint) error {
	expectedClientSeq := checkpoint.ClientSeq + 1
	actualFirstClientSeq := changes[0].ClientSeq()

	if expectedClientSeq < actualFirstClientSeq {
		return fmt.Errorf(
			"ClientSeq is not sequential with DocInfo.Checkpoint.ClientSeq (expected: %d, actual: %d) : %w",
			expectedClientSeq,
			actualFirstClientSeq,
			ErrClientSeqNotSequentialWithCheckpoint,
		)
	}
	return nil
}

these two case return connect.CodeInvalidArgument

// server\rpc\connecthelper\status.go

var errorToConnectCode = map[error]connect.Code{
	
       ...

	packs.ErrClientSeqNotSequentialWithCheckpoint: connect.CodeInvalidArgument,
	packs.ErrClientSeqInChangesAreNotSequential:   connect.CodeInvalidArgument,

}

...

1.3 Case 3: The ClientSeq in changes cannot be less than the ClientSeq in the Server's ClientInfo

This was already implemented, and it simply logs a warning. It returns OK as specified in the current issue.

pushpull.go, function pushChanges()

    ...
	cp := clientInfo.Checkpoint(docInfo.ID)

	var pushedChanges []*change.Change
	for _, cn := range reqPack.Changes {
		if cn.ID().ClientSeq() > cp.ClientSeq {
			serverSeq := docInfo.IncreaseServerSeq()
			cp = cp.NextServerSeq(serverSeq)
			cn.SetServerSeq(serverSeq)
			pushedChanges = append(pushedChanges, cn)
		} else {
			logging.From(ctx).Warnf(
				"change already pushed, clientSeq: %d, cp: %d",
				cn.ID().ClientSeq(),
				cp.ClientSeq,
			)
		}

		cp = cp.SyncClientSeq(cn.ClientSeq())
	}
    ...

1.4 Case 4: The ServerSeq in the request cannot be greater than the ServerSeq maintained by the server.

This was also already implemented. In the code below, initialServerSeq refers to the ServerSeq maintained in the Document Info.

Q. Currently, it returns connect.CodeFailedPrecondition instead of connect.CodeInvalidArgument as specified in the issue. Is this appropriate?

pushpull.go pullPack

    ...
    if initialServerSeq < reqPack.Checkpoint.ServerSeq {
		return nil, fmt.Errorf(
			"serverSeq of CP greater than serverSeq of clientInfo(clientInfo %d, cp %d): %w",
			initialServerSeq,
			reqPack.Checkpoint.ServerSeq,
			ErrInvalidServerSeq,
		)
	}
    ...

1.5 Test Code

The test code is located in server/packs/packs_test.go. It covers the following four scenarios:

// server/packs/packs_test.go

func Test(t *testing.T) {
	t.Run("push/pull sequential ClientSeq test (happy case)", func(t *testing.T) {
		RunPushPullWithSequentialClientSeqTest(t)
	})

	t.Run("push/pull not sequential ClientSeq test", func(t *testing.T) {
		RunPushPullWithNotSequentialClientSeqTest(t)
	})

	t.Run("push/pull ClientSeq less than ClientInfo's ClientSeq (duplicated request)", func(t *testing.T) {
		RunPushPullWithClientSeqLessThanClientInfoTest(t)
	})

	t.Run("push/pull ServerSeq greater than DocInfo's ServerSeq", func(t *Testing.T) {
		RunPushPullWithServerSeqGreaterThanDocInfoTest(t)
	})
}

1. happy case: Sequential ClientSeq in Client Request
2. Not Sequential with DocInfo.Checkpoint.ClientSeq
3. Not Sequential ClientSeq in Client Request Changes
4. duplicated request: ClientSeq in Request is less than ClientInfo's ClientSeq (But No Error)
5. ServerSeq in Request is greater than DocInfo's ServerSeq

2. Which issue(s) this PR fixes:

resolves #805

3. Special notes for your reviewer:

Questions 1 through 3 are addressed in the case descriptions above:

1. Case 1: The function validateClientSeqSequential() verifies that the Changes in the request are sequential. It does not validate whether the first Change's ClientSeq differs by 1 from the ClientSeq in the existing ClientInfo. Should this be added?

2. Case 1: Should we remove the TODO comment in the PushPull function in server/packs/packs.go?

3. Case 3: The issue specifies that malicious ServerSeqs should return connect.CodeInvalidArgument, but currently, it returns connect.CodeFailedPrecondition. Is this appropriate?

4. If client actions are required in response to these scenarios, we should discuss how to handle them.

   • e.g., The client should be aware that Changes are not applied if the request is not sequential.

4. Does this PR introduce a user-facing change?:

5. Additional documentation:

Here, I have attached the questions I posted on the issue and the answers I found. I hope this will be helpful to those studying ClientSeq and ServerSeq. If there are any mistakes, please let me know.

I have also attached the responses from @sejongk to my questions. I learned a lot from his answers, and I am very grateful to him.

5.1 My Question And Answer

Hello,
I have a few questions as I work on understanding the content to resolve the issue.

• Issue: Validate Checkpoint in ChangePack for PushPull API requests

Q1. Where do the server's ClientSeq and ServerSeq come from?

To resolve this issue, I thought it would be sufficient to compare the ClientSeq and ServerSeq of the client with those maintained by the server.

Is it correct that the server's ClientSeq and ServerSeq can be obtained from the Checkpoint of ClientInfo, which is derived from the ClientId in the Request Message?

Q2. Where do the ClientSeq and ServerSeq sent by the client come from?

The ChangePack in the Request's Msg contains a Checkpoint and an array of Changes.
The Checkpoint holds its ClientSeq and ServerSeq, and each Change in the array also has its ClientSeq and ServerSeq.
According to the issue, it mentions "so Change.ID.Checkpoint.ClientSeq should increment sequentially by one." Does this mean that the ClientSeq of the client should be the sequential values of each Change, and these should be validated against the server's ClientSeq (rather than the ClientSeq in the Checkpoint of the ChangePack)?

And regarding "Checkpoint.ServerSeq in the request ChangePack for PushPull API cannot be greater than the server's Checkpoint.ServerSeq since it is set when the server saves the Change to the database."

Should the client's ServerSeq be obtained from the ServerSeq in the Checkpoint of the ChangePack in the Msg (not the ServerSeq of each individual Change)?

Answer 1, 2

1. The server's ServerSeq can be found in DocInfo. Since the ServerSeq is updated every time a Change from various clients is applied, it is managed at the Document level.
2. The server's ClientSeq can be found in the Checkpoint of ClientInfo. Since the server needs to know the last ClientSeq sent by the client, it is managed at the ClientInfo level.
3. The client's ClientSeq is assigned by the client for each Change and is sequentially included in Changes. The ClientSeq can be found in both the Checkpoint and the Changes.
4. The client's ServerSeq can be found in reqPack.Checkpoint.ServerSeq. The client retains the last ServerSeq received during synchronization and includes it in the Checkpoint when sending a Request.

Q3. How is a "wrong" ClientSeq determined?

You mentioned that Change.ID.Checkpoint.ClientSeq should increment sequentially by one. Does this mean that the following scenarios are considered wrong?

1. If the ClientSeq of each Change in the ChangePack sent by the client is not sequential, the request is wrong.
2. If the ClientSeq in the CheckPoint sent by the client is ≤ the _ClientSeq in clientInfo, it is wrong.

Answer 3

1. Correct. There was an issue during the process of saving the Changes.
2. Correct. This corresponds to a duplicate request.

Q4. How is a duplicate request determined?

Would it be correct to consider the scenario where the ClientSeq sent by the client is equal to the _ClientSeq in clientInfo as a duplicate request due to network delay? I seem to be missing the criteria for determining a "duplicate request."

Answer 4

Answered in Answer 3.

Q5. How is a wrong ServerSeq determined?

Is it wrong when the ServerSeq sent by the client is greater than the ServerSeq obtained from ClientInfo on the server?

Answer 5

If the ServerSeq sent by the client is greater than the ServerSeq found in DocInfo on the server, it is a malicious request. This is because the ServerSeq held by the client is initially provided by the server. Since the ServerSeq increases sequentially, the client's ServerSeq should never be larger.

Q6. Where should the validation take place?

I believe it should occur in the PushPullChanges function in yorkie_server.go. Should I declare the validate function and error directly in yorkie_server.go, or should I create a validator file in the rpc package and declare the validate function and error there?

Answer 6

I was curious whether a separate object or layer was needed for validation. Based on Yorkie's existing validation methods, it seems unnecessary.

5.2 sejongk's Answer

Understanding the concept of a Checkpoint seems crucial for this issue. I searched for relevant design documents and found a document on GC garbage-collection.md , though it might be good to add more detailed information about Checkpoints there or even create a separate Checkpoint document in the future.

You may already be familiar with Checkpoints, but I'll share what I reviewed today:

• The Yorkie server imposes sequential consistency on the Changes generated by multiple clients. In other words, the server assigns the id of the Changes received during the push process with a global counter called server_seq, which is incremented by 1. The docInfo.SeverSeq corresponds to this global counter.
• Later, during the pull process, the server sends the Changes that the client has not yet received, along with the current server_seq that has been incremented up to that point. This server_seq serves to inform the client of how far it has received the Changes.
• When the client performs a pushpull, it sends the server_seq it previously received to the server in reqPack.Checkpoint.ServerSeq, notifying the server of how far it has received and prompting the server to send the Changes corresponding to the subsequent server_seq.
• The server holds the client_seq on the client side as clientInfo.Checkpoint(docInfo.ID).ClientSeq, which is used to validate the client_seq of the Changes.
  • ChangePack is used as a group for buffering Changes. The client_seq within the ChangePack must be sorted and incremented by 1 sequentially. This is because if a Change is lost due to network issues, it can be detected through the client_seq. This is similar to how sequence numbers are assigned to each packet in TCP.
  • It also solves the problem of duplicate Changes, as the server currently checks whether the client_seq is smaller than the client_seq the server holds (if cn.ID().ClientSeq() > cp.ClientSeq).
• The Checkpoint logic is crucial in GC, but since it's not the main point of this issue, I left it out. min_synced_seq is one of the core elements in GC.

After reviewing the above, it would be helpful to look at the diagrams explaining the Checkpoint flow at the following links:

• https://miro.com/app/board/o9J_kidjCtk=/?moveToWidget=3458764571662877621&cot=14
• https://miro.com/app/board/o9J_kidjCtk=/?moveToWidget=3458764554509936673&cot=14

You may want to review the code in these files:

• server/packs/packs.go/func PushPull
• server/packs/pushpull.go/func pushChanges
• server/packs/pushpull.go/func pullChanges

6. Checklist:

• validate ClientSeqs in the request sent to the PushPull API are sequential.
• validate ClientSeq in the request sent to the PushPull API cannot be less than the ClientSeq in the Server's ClientInfo. This means duplicate requests.
• validate ServerSeq in the request sent to the PushPull API cannot be greater than the ServerSeq maintained by the server. This means malicious requests.
• Write test code

Summary by CodeRabbit

• New Features

  • Enhanced error handling for client sequence validation during document synchronization.
  • Introduced new error types for non-sequential client sequences.

• Bug Fixes

  • Improved control flow in document processing to prevent invalid changes from being processed.
  • Added specific error mappings to enhance feedback during connection issues related to client sequence validation.

• Tests

  • Added comprehensive unit tests for validating client sequence handling in push and pull operations.